Map Ribo-TISH's colon-qualified ORF types on their location - #12613
Map Ribo-TISH's colon-qualified ORF types on their location#12613FelixKrueger wants to merge 4 commits into
Conversation
Ribo-TISH qualifies a positional label after a colon rather than replacing it: Novel:CDSFrameOverlap, 3'UTR:CDSFrameOverlap, Internal:CDSFrameOverlap, Novel:Known. CLASS_TOKENS holds only the bare locations and classify() matched the whole token, so every one of those fell through unmatched. Since nf-core#12498 made an unmatched token fatal, real Ribo-TISH output now aborts the process — four nf-core/riboseq pipeline tests failed this way on chr20 data, with 3 to 33 offending rows each. Match on the part before the first colon. orf_class is positional, and CDSFrameOverlap describes a frame relationship rather than a position, so the qualifier does not belong in the class; orf_type_native already carries the full token verbatim, so nothing is lost and the harmonisation stays auditable. No CLASS_TOKENS key contains a colon, so this cannot shorten a token meant to match whole, and no new vocabulary entries are needed: the four tokens resolve to novel_u, dORF and intORF through the existing table. The module tests missed this because their fixtures use bare tokens; only real Ribo-TISH output produces the composite form. The added test uses the four observed tokens and fails without this change with the same message the pipeline produced.
The workflow is registered (id 328515623) from its 08-06 run, so the dispatch API resolves it by numeric ID and workflow_dispatch is enough to start it on demand. Dispatch by filename would not work: that lookup requires the file to be on the default branch, and this one deliberately never reaches master. The push trigger is dropped rather than kept alongside. Pushing this commit would otherwise have had a chance of starting the four 45-minute jobs, and they cannot pass until nf-core/modules#12613 has merged and the SHAs are re-bumped.
The original checks for this head wedged during the 2026-08-06 Actions incident:
queued with zero jobs created, and refusing both cancel ("already completed")
and rerun ("already running"). No API path revives them, so a new head is the
only way to get CI. The tree is unchanged, so the local 11/11 verification still
describes exactly this content.
|
CI is red here but none of it belongs to this PR: all four failures are Evidence (AI-assisted)The four failing checks, out of 72 check runs across 7 suites:
PIRATE's diff shows the tool deciding it failed, not snapshot drift — expected outputs absent, failure marker present: This module's tests. Why the head moved. The five original checks on Local verification, unchanged from the PR description: 11/11 including a negative control — reverting only |
|
Merging |
| """ | ||
| input[0] = channel | ||
| .of( | ||
| 'Tid\tGid\tGenomePos\tTisType\tAALen\tFisherPvalue', |
There was a problem hiding this comment.
Do you have a real input we could use as a proper fixture in test-datasets? The existing one (genomics/homo_sapiens/riboseq_expression/orf_predictions/sample1.ribotish.pred.txt) obviously isn't representative enough, so we should consider adding another one rather than embedding dummy data here.
There was a problem hiding this comment.
Yes — real output with these tokens exists, so this needn't stay synthetic. The composite labels are what made a pipeline-level snapshot-recording run fail on genuine chr20 Ribo-TISH output, with this distribution:
Novel:CDSFrameOverlap 29
Novel:Known 4
3'UTR:CDSFrameOverlap 1
Internal:CDSFrameOverlap 1
That is also why the module tests missed the gap — the existing fixture only contains bare tokens, exactly as you say.
Two choices I would rather you made before I open the test-datasets PR, since both affect other modules:
- A second file, or make the existing one representative? My instinct is to add a new file (
sample1.ribotish.composite.pred.txtor similar) rather than modifysample1.ribotish.pred.txtin place. Editing a shared fixture is what briefly brokecustom/orfcollapsewhen Refresh the custom/orfcollapse fixture for the positional orf_class vocabulary test-datasets#2193 merged — there is no version gate, so every consumer picks the change up immediately and only notices when something next touches it. But if you would rather the existing fixture simply became representative, that is fine too and I will sequence the module PRs behind it. - Region. The existing
sample1.*family is chr20, and real composite tokens are plentiful there, so the family can stay on one region unless you want otherwise.
Once you have picked, the order is test-datasets first, then this PR swaps the inline rows for the fixture. Happy to do both.
There was a problem hiding this comment.
We try not to overwrite files in test-datasets, they should really be write-once (though it's patchily followed). Add a new one.
There was a problem hiding this comment.
Added as a new file in nf-core/test-datasets#2208, generated by running the pipeline on its own chr20 data in extended-ORF mode. Worth flagging: real output has seven colon-qualified forms, not the four we knew about -- 5'UTR:Known, 5'UTR:CDSFrameOverlap and Truncated:Known also occur, and all seven already resolve through the existing table. I will point the test at the fixture and drop the inline rows once that merges.
| part before the first colon describes where the ORF sits, and `orf_class` | ||
| is positional, so the qualifier is dropped here and preserved verbatim in | ||
| `orf_type_native`. No CLASS_TOKENS key contains a colon, so this cannot | ||
| shorten a token that was meant to match whole. |
There was a problem hiding this comment.
Can we reduce the AI historical narrative in the comments? I get this problem a lot, but comments should be stand-alone and represent the current state, not reference a previous state you're fixing.
There was a problem hiding this comment.
Trimmed in 45d5122 — comment-only across the three files, and no executable change: orfnormalise.py's token stream is identical apart from the docstring literal itself.
classify()docstring: the paragraph describing what the previous code did is gone. It now states only that the part before the first colon is matched and that the full token survives inorf_type_native.CLASS_TOKENSheader: the Ribo-TISH narrative is replaced by the single constraint a future editor of that table actually needs —No key may contain a colon: classify() matches only the part before one.That invariant is load-bearing (a colon-bearing key would be silently truncated by the split), so I kept it, but moved it onto the table it constrains rather than leaving it as a justification in the docstring.meta.ymland the new test assertion: same treatment.
One thing I left alone: there is a pre-existing instance of exactly this pattern a few lines above, from #12498 — Substring matching mis-fired on the overlap forms, because "uorf" is a substring of "overlap_uorf". Happy to fold that in here if you'd like it gone now; I left it out so this PR stays scoped to comments it introduced itself.
Review asked for comments that stand alone rather than describe what the previous code did. The classify() docstring, the CLASS_TOKENS header, meta.yml and the new test now state current behaviour only. The colon-free-key invariant is kept, moved onto the table it constrains so it is read when that table is edited. Comment-only: orfnormalise.py's token stream is unchanged apart from the docstring literal (5877 tokens before and after, one differing).
2 participants
Ribo-TISH qualifies a positional ORF-type label after a colon rather than replacing it —
Novel:CDSFrameOverlap,3'UTR:CDSFrameOverlap,Internal:CDSFrameOverlap,Novel:Known.CLASS_TOKENSholds only the bare locations andclassify()matched the whole token, so all of those fell through unmatched; since #12498 made an unmatched token fatal, real Ribo-TISH output now aborts the process. This matches on the part before the first colon instead.orf_classis positional andCDSFrameOverlapdescribes a frame relationship rather than a position, so the qualifier does not belong in the class — andorf_type_nativealready carries the full token verbatim, so nothing is lost. No new vocabulary entries are needed.Found by four nf-core/riboseq pipeline tests failing on real chr20 data, with 3–33 offending rows each.
Verification notes (AI-assisted)
Why the existing tests didn't catch it. The module fixtures use bare tokens; only genuine Ribo-TISH output produces the composite form, so #12498 was 63/63 green while this was already broken for real data. The added test uses the four tokens observed in production and fails without this change with the same message the pipeline produced:
Why location-only, rather than enumerating the composites. Taking the location keeps
orf_classpurely positional, which is the invariant #12498 established when it split length out intois_smorf. Enumerating<location>:<qualifier>pairs would need a decision per combination and would reintroduce the brittleness that exact-token matching removed. The four observed tokens resolve through the existing table with no additions:orf_classNovel:CDSFrameOverlapNovelnovel_uNovel:KnownNovelnovel_u3'UTR:CDSFrameOverlap3'UTRdORFInternal:CDSFrameOverlapInternalintORFWhy the split is safe for every caller. Checked all 52
CLASS_TOKENSkeys across the five callers: none contains a colon, so splitting cannot shorten a token that was meant to match whole. A comment on the table records that constraint for anyone adding keys later.A location that matches nothing still fails, so the guard #12498 added keeps its value — only the qualifier is permitted to be unknown, which is what makes this robust to Ribo-TISH adding qualifiers.
nf-test test modules/nf-core/custom/orfnormalise— 11/11 pass locally (10 pre-existing plus the new one).